-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: have JsonUtil support parsing String value as number #8711
Conversation
cc @ejona86 |
} | ||
if (value instanceof String) { | ||
try { | ||
return Double.parseDouble((String) value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're returning wrapped Double
, would it make sense to use Double.valueOf
instead?
return Double.parseDouble((String) value); | |
return Double.valueOf((String) value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll keep these as-is as they are nits/minor.
return Double.parseDouble((String) value); | ||
} catch (NumberFormatException e) { | ||
throw new IllegalArgumentException( | ||
String.format("value '%s' for key '%s' is not a double", value, key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking how to express that the string representation failed. Maybe
String.format("value '%s' for key '%s' is not a double", value, key)); | |
String.format("string value '%s' for key '%s' is not a double", value, key)); |
or
String.format("value '%s' for key '%s' is not a double", value, key)); | |
String.format("value '%s' for key '%s' is not a string representation of a double", value, key)); |
*/ | ||
@Nullable | ||
public static Double getNumber(Map<String, ?> obj, String key) { | ||
public static Double getNumberAsDouble(Map<String, ?> obj, String key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be out of scope, but what I find really strange is that we mix getting an object from a map, and converting it to something. Why not
public static Double getNumberAsDouble(Object value) { ... }
public static Double getNumberAsDouble(String value) { ... }
and then call it as, for example, JsonUtil.getNumberAsInteger(retryPolicy.get("maxAttempts"));
Seems like cleaner approach, easier to read, and easier to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Convenience. We need clear error messages when a field is not present. Getting a field from an object is the strongly-common case, so it would be worth the duplicate method
- The only other way to get a number is from a list, and we tend to have specialized helpers for that (
getListOfObects
,getListOfStrings
). It is rare (currently non-existent) to need to mix-and-match types within a list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like this?
@Nullable
public static Double getNumberAsDoubleFromObj(Map<String, ?> obj, String key) {
try {
return getNumberAsDouble(getFromObj(obj, key));
} catch (NumberFormatException e) {
throw new IllegalArgumentException(String.format("key '%s': %s", key, e.getMessage()));
} catch (IllegalArgumentException e) {
throw new IllegalArgumentException(
String.format("key '%s' in '%s': %s", key, obj, e.getMessage()));
}
}
public static Double getNumberAsDouble(Object value) {
if (value instanceof Double) {
return (Double) value;
}
if (value instanceof String) {
try {
return Double.parseDouble((String) value);
} catch (NumberFormatException e) {
throw new NumberFormatException(
String.format("value '%s' is not a string representation of a double", value));
}
}
throw new IllegalArgumentException(String.format("value '%s' is not a number", value));
}
static Object getFromObj(Map<String, ?> obj, String key) {
assert key != null;
if (!obj.containsKey(key)) {
return null;
}
return obj.get(key);
}
Pros:
- we get separation of concerns
getFromObj
(name tentative) can be reused in allgetNumberAs*FromObj
getNumberAs*
can be reused elsewheregetNumberAs*
are not nullable- straightforward tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what getFromObj()
is providing, as obj.get(key)
return null
for non-present keys. This isn't Python. Note, if we had such a method it should throw an exception instead of returning null. The current methods consider null to be just another type and so fall-through to catch-all exceptions.
Nobody should use getNumberAsDouble(Object)
; "you're doing it wrong" and the caller will probably have poor error handling. I'm going to argue pretty strongly that we should not have the method.
The method can exist, but it can be private. So this is really not something that needs to be debated now. As I said, the main strong possibility for reuse is with arrays of all of the same time. Maybe some day we'll need per-index type management, but I think that would just be a third method when we need it; I question if we'll ever need it.
There are no nullability checks in our builds, and nullability is known to be wrong many places and overall normally missing. Nullability annotations are a non-required annotation that just acts as a part of the javadoc, essentially. Nullability is good and fine, but it doesn't help anyone catch bugs any more than documentation.
It's also not clear to me why we need so much exception handling in getNumberAsDoubleFromObj()
.
Renaming stuff to include *FromObj
feels more like bikeshedding to me, or fixing a concern that the current callers don't suffer from. Some of those changes may be fine, but not as part of this change. That would be a separate refactor to the callers that should not change existing behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what getFromObj() is providing, as obj.get(key) return null for non-present keys. This isn't Python. Note, if we had such a method it should throw an exception instead of returning null. The current methods consider null to be just another type and so fall-through to catch-all exceptions.
I think you missed the assert there. getFromObj
does exactly what happens right now, it's a carbon copy of the first 5 lines of nearly every get*
method in this class. So at least it's DRY.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'm surprised this has become a discussion point instead of something like implementing getNumberAsInteger() via getNumberAsLong() (or shared helper).
To clarify: you mean this as an argument for the private methods that we discussed above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, those private methods were for something different. I'm talking about how getNumberAsInteger()
could be implemented as:
Long l = getNumberAsLong(obj, key);
if (l == null)
return null;
int i = l.intValue();
if ((long) i != l.longValue())
throw ...;
return i;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that getNumberAsLong
produces slightly different error messages, f.e: "value '%s' for key '%s' is not a long integer"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was more of a sketch. And you have to figure out if the cost of merging is worth the benefit. I'm fine with it both ways (I think there are some ways that may not be too ugly to do it). Honestly, I'd be tempted to do it mostly because s/long/int/ is so error prone when comparing that logic.
@sergiitk and I talked. We're actually more on the same page than it seemed. The root of this is chain was really spawned by the tests, and I agree the tests exhibit some code smells that are a bit hard to track down the source for and the suggestions would improve things (for the tests).
The end result is probably not to change the API now. But we were also marveling at how many different issues something this simple brought up. Things could be improved with just changes to the tests, but we're probably not going to be overly critical because some of this should have been noticed with tests (that don't exist) introduced when JsonUtil was introduced. But we're also able to see "how we ended up here."
We should continue thinking about improvements to this area, but won't see changes right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree there is space for improvement, but that's not something we should do in this PR.
assertThat(JsonUtil.getNumberAsDouble(map, "k3")).isEqualTo(3D); | ||
assertThat(JsonUtil.getNumberAsInteger(map, "k3")).isEqualTo(3); | ||
assertThat(JsonUtil.getNumberAsLong(map, "k3")).isEqualTo(3L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd recommend smaller unit tests for each individual method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per-IRL discussion, the only suggestion I see as important is the one with adding test for the special cases: #8711 (comment)
Everything else is nits/minor suggestion. Happy to approve as is.
As documented in https://developers.google.com/protocol-buffers/docs/proto3#json,
the canonical proto-to-json converter converts int64 (Java long) values to string values in Json rather than Json numbers (Java Double). Conversely, either Json string value or number value are accepted to be converted to int64 proto value.
To better support service configs defined by protobuf messages, support parsing String values as numbers in
JsonUtil
.